-
-
Notifications
You must be signed in to change notification settings - Fork 899
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Use cached image when creating single source TiledAtlas if available #2348
Conversation
(await Flame.images.load(tiledImage.source!, key: key)).clone(); | ||
|
||
final image = (await Flame.images.load(tiledImage.source!)).clone(); | ||
Flame.images.add(key, image); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There will also be two image objects added to the cache now right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right. But they share an image reference.
At present, TiledAtlas
has a rule for storing keys.
- No image:
atlas{empty}
=> Actually this is not used as a key because there is no image to store. - A image:
atlas{$source}
- Images:
atlas{$source1,$source2}
To keep this rule, I added this: Flame.images.add(key, image);
.
But If there is no need to keep this rule, the line can be eliminated.
I prefer removing this but it's totally on your call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the image is cloned before it is added to the cache again they don't share the reference.
I don't think there is a need to keep that rule, what do you think? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the image is cloned before it is added to the cache again they don't share the reference.
Oh, I was totally missed it. Yes, you are right again!
I don't think there is a need to keep that rule, what do you think? :)
I absolutely agree with you since there is a key
property to TiledAtlas
so that it could get a cached image if anyone wanted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, do you think the clone
is needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, feels like there must have been a reason for someone to put the clone there? Can you check who did it and we can ask? I'm currently on my phone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure.
Hi @jtmcdole, can I ask a reason for the clone
exists? I could roughly assume but curious about the exact reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@spydon Maybe he's unavailable now. We should leave the clone
and ask him later :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can keep using the image so long as it is in the cache. We don't go back to the cache each time we want to reference the image and instead use final Image? atlas;
. If the cache is cleared for any reason, it disposes of the image and we now have an invalid image.
I remember there being a crash due to this... there should have been a test as well :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably some of your use cases are evicting the cache. Thanks for the answer 😄
Description
Detailed use-case examples are described in #2337.
As-is
To-be
Checklist
docs
and added dartdoc comments with///
.examples
ordocs
.Breaking Change?
Related Issues
Closes: #2337